Fix invalid timings in span events #4486
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4486 +/- ##
==========================================
+ Coverage 92.80% 92.82% +0.01%
==========================================
Files 328 328
Lines 9512 9519 +7
Branches 2047 2050 +3
==========================================
+ Hits 8828 8836 +8
+ Misses 684 683 -1
|
|
@MSNev , @martinkuba , @dyladan , can you review this ? |
| span.addEvent(performanceName, entries[performanceName]); | ||
| let perfTime = entries[performanceName]; | ||
| const refName = refPerfName || PTN.FETCH_START; | ||
| // Use a reference time whcih is the earliest possible value so that the performance timing are earlier can be corrected to this reference time |
There was a problem hiding this comment.
This might come down to opinions but I think just dropping (ignoring) timings that are earlier than expected value is better and matches the intention of 0 values (= value not available), while this converts it to a value that makes sense but not real
eg. considering 2 easily known cases of 0-value:
- secureConnectionStart is 0 when loading an insecure (http) resource because you don't have a secure connection there
- most of the timings when a cross-origin request is done and there's no Timing-Allow-Origin header
This would also make it easier to avoid buggy processing of data - eg. if someone processes incoming data to get the connecting time per url by connectEnd - connectStart, they would always get 0ms to connect (which is a valid value when re-using existing connection!) for non-TAO header cross-origin requests (as this would fix them to equal fetchStart), while if it's missing there'd be an easy way to know that you shouldn't calculate a value from this span
There was a problem hiding this comment.
Yes, we can discuss on which option to take, either avoiding reporting the invalid timing, or correcting it as done here. In the current condition, all timings are getting reported whether valid or invalid.
There was a problem hiding this comment.
I would also lean toward conditionally adding the timing only if it's valid, and if it's not valid, don't add it. This helps prevent skewing of results.
There was a problem hiding this comment.
@t2t2 , @JamieDanielson I have made the changes to conditionally add the timings
The change in open-telemetry/opentelemetry-js#4486 means that a addSpanNetworkEvent() in v1.24.0 and later might get dropped -- if its time value is before the fetchStart time. Typically this happens if the event time value is 0.
…nts (#2145) The change in open-telemetry/opentelemetry-js#4486 means that a addSpanNetworkEvent() in v1.24.0 and later might get dropped -- if its time value is before the fetchStart time. Typically this happens if the event time value is 0.
* fix: use reference value to avoid invalid timings in span * add comment, add to changelog * donot report invalid timing in span * lint fix * fix failing test * fix failing test 2 * Add tests * add removed test * suggested changes --------- Co-authored-by: Marc Pichler <[email protected]>
Which problem is this PR solving?
Fix invalid timings in span events
Short description of the changes
add reference time to check if the value to be reported for the event is sane (use max of time and reference time)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Checklist: